Skip to content

Fix sum by name in custom metrics - #24673

Open
gabotechs wants to merge 1 commit into
mainfrom
gabotechs/fix-sum-by-name
Open

Fix sum by name in custom metrics#24673
gabotechs wants to merge 1 commit into
mainfrom
gabotechs/fix-sum-by-name

Conversation

@gabotechs

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

Something small I spotted. MetricValue::sum_by_name seems to be ignoring custom metrics, while these metrics also have a valid name.

What changes are included in this PR?

The fix and one unit test.

Are these changes tested?

Yes

Are there any user-facing changes?

People can now call sum_by_name in custom metrics

@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label Aug 25, 2026
@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.44%. Comparing base (1064661) to head (c50f325).

Files with missing lines Patch % Lines
datafusion/physical-expr-common/src/metrics/mod.rs 78.37% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24673   +/-   ##
=======================================
  Coverage   81.44%   81.44%           
=======================================
  Files        1118     1118           
  Lines      399602   399638   +36     
  Branches   399602   399638   +36     
=======================================
+ Hits       325458   325505   +47     
+ Misses      55146    55136   -10     
+ Partials    18998    18997    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nuno-faria nuno-faria left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gabotechs.

self.sum(|m| match m.value() {
MetricValue::Count { name, .. } => name == metric_name,
MetricValue::Time { name, .. } => name == metric_name,
MetricValue::OutputRows(_) => false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if in the future we should also add metrics like OutputRows to this (i.e., metric_name == "output_rows").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants